Bugfix: Apply Updaters to Graph Edges During Edge Creation #3836
Conversation
|
Addresses #3584 |
|
Thanks for your efforts! I have not reviewed your changes made to IIRC, there was a reason why I went with one global graph-updater instead of one updater per edge -- perhaps it was performance. Things have changed quite substantially since the initial implementation though, so perhaps this isn't really relevant anymore; I'll run some tests. I actually also think that I'll look into this some more. The failing tests are likely due to a different version of cairo on your end. If this is the case, I can regenerate it for you to make the pipeline pass. Again, thanks for your efforts! |
|
Not sure why that last test I wrote keeps acting up. It's failing locally for me now, even right after a --set_test. I'll have to look into that. I see what you're saying with tracking sibling mobjects being just as weird if not weirder than applying to a parent. If you want to go down the road of applying the update from the AnimationGroup/Graph, the change that looks interesting to me is removing the update suspension from AnimationGroup's begin(), since all the child animations suspend their own mobjects' updates anyway, and suspending animation on the group/parent overall is what blocks updates to parent mobjects like the Graph. I made a quick and dirty draft PR of this #3838, and it seems to pass my original test (creating an edge while moving a vertex), without breaking any existing tests. |
tlcyr4
force-pushed
the
graph-concurrent
branch
2 times, most recently
from
a369c19
to
5f5dd59
Compare
|
In the meantime, I just pushed a lighter version of this change that still cleans up the config dict shaping, which fixes two problems where
|
JasonGrace2282
added
the
pr:bugfix
for more information, see https://pre-commit.ci
|
Before merging, I would prefer the docs build passes - does the example need to be changed? |
Overview: What does this pull request change?
This fixes a problem where edges added to a graph after the graph's initial creation don't track their vertices' position until the end of the creation animation.
Test.mp4
The issue here is a consequence of how parallel updates work during some animations like Create, where we create a defensive copy of the mobject and draw a progression of interpolations starting from a blank canvas and working back toward that defensive copy.
If there's another set of updates that we need to apply to this mobject in tandem with the animation, we apply them to the defensive copy, which was created just before updates were suspended on the original mobject.
This can cause problems when the mobject has a parent mobject that's also involved in the animation.
First, in this Graph case, the Edge's updater was actually attached the the parent Graph (it was one big updater that kept all the Graph's Edges on their vertices). The animation that applied to the Graph was an AnimationGroup, which doesn't directly call updaters on its target mobject, and instead relies on its child Animations to apply updaters on their targets. However, the child Animation, a Create(Line), applied to the Edge rather than the Graph, so nothing was calling updaters for the Graph itself until the very end of the animation.
Second, when updates were suspended for the Graph, that operation was applied recursively to all submobjects of the Graph. Since this happened before the defensive copy of the Edge was created, that defensive copy inherited the "updating_suspended=True" flag, which blocked all updates from applying to the Edge until the end of the overall animation.
To resolve the above, this PR makes two changes:
This doesn't help with other cases where we attach a child's updater to the parent, but the general idea of an updater affecting a mobject other than the one it's directly attached to, regardless of parental relationship, seems like a whole can of worms to me.
Motivation and Explanation: Why and how do your changes improve the library?
bugfixes
Links to added or changed documentation pages
Should not affect any documentation.
Further Information and Comments
Reviewer Checklist